feat(apollo-react): shared guardrail definitions layer and useGuardrailDefinitions [AL-574] - #1139
andreizdrali-uipath wants to merge 1 commit into
Conversation
|
Apollo Coded App preview deployments are ready.
|
Dependency License Review
License distribution
Excluded packages
|
There was a problem hiding this comment.
🟡 Changes recommended
There are confirmed behavioral bugs in the new code (unexpected refetch behavior and render-phase state updates) that should be fixed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds a shared “guardrail definitions” layer under packages/apollo-react/src/canvas/components/Guardrails/ that parses the /api/execution/guardrails/definitions payload, enriches it with canonical (Lingui-backed) display copy, and exposes a useGuardrailDefinitions hook as the seam between host transport and GuardrailBuilder. It also extends apollo-wind’s metadata forms to support tooltips and a new string-list field type used by guardrail parameter editors.
Changes:
- Introduces wire types + zod-based non-throwing parsing, pure enrichment, and a
useGuardrailDefinitionshook for fetching/composing guardrail definitions. - Moves canonical validator copy into the canvas Lingui catalog and adds parity tests against Flow/Agents baselines.
- Enhances
apollo-windforms/UI withInfoTooltip,string-listfield support, andaria-invalidstyling for select/textarea.
File summaries
| File | Description |
|---|---|
| pnpm-lock.yaml | Locks new deps added for guardrails UI and a11y testing. |
| packages/apollo-wind/src/index.ts | Re-exports new forms/types (MetadataFormProps, useWatch, StringListField*) and InfoTooltip. |
| packages/apollo-wind/src/components/ui/textarea.tsx | Adds aria-invalid error styling to textarea. |
| packages/apollo-wind/src/components/ui/select.tsx | Adds aria-invalid error styling to select trigger. |
| packages/apollo-wind/src/components/ui/info-tooltip.tsx | Adds reusable info-icon tooltip component for form labels. |
| packages/apollo-wind/src/components/ui/info-tooltip.test.tsx | Adds a11y + behavior tests for InfoTooltip. |
| packages/apollo-wind/src/components/ui/index.ts | Exports info-tooltip (and reorders a couple exports). |
| packages/apollo-wind/src/components/forms/validation-converter.ts | Extends schema conversion to treat string-list as an array type. |
| packages/apollo-wind/src/components/forms/string-list-field.tsx | Implements the new string-list field editor and formatTemplate helper. |
| packages/apollo-wind/src/components/forms/metadata-form.stories.tsx | Adds story demonstrating string-list + tooltip + controlled-host seam. |
| packages/apollo-wind/src/components/forms/index.ts | Exposes new forms APIs (controlled seam types, string-list exports, useWatch). |
| packages/apollo-wind/src/components/forms/form-schema.ts | Adds tooltip metadata, textarea constraints, multiselect copy overrides, and string-list field metadata/type. |
| packages/apollo-wind/src/components/forms/field-renderer.tsx | Renders required indicator + optional tooltip, wires htmlFor/id, and passes aria-invalid to select/textarea/multiselect. |
| packages/apollo-react/src/test/setup.ts | Registers jest-axe matchers for Vitest suites. |
| packages/apollo-react/src/i18n/index.ts | Exports getPreImportedMessages helper for hosts merging catalogs. |
| packages/apollo-react/src/canvas/locales/en.json | Adds guardrails chrome strings + canonical validator/parameter/option copy (English). |
| packages/apollo-react/src/canvas/components/index.ts | Exports the Guardrails canvas family from the canvas components barrel. |
| packages/apollo-react/src/canvas/components/Guardrails/utils.ts | Adds parameter seeding/sync/validation helpers for guardrail parameters. |
| packages/apollo-react/src/canvas/components/Guardrails/use-guardrail-definitions.ts | Adds useGuardrailDefinitions hook (fetch + parse + enrich + refetch). |
| packages/apollo-react/src/canvas/components/Guardrails/types.ts | Defines guardrail parameter and form prop types for the validator editor surface. |
| packages/apollo-react/src/canvas/components/Guardrails/render-parameter-bridge.tsx | Bridges host renderParameter overrides into MetadataForm custom components via context. |
| packages/apollo-react/src/canvas/components/Guardrails/index.ts | Public exports for the guardrails family, including the new definitions layer APIs. |
| packages/apollo-react/src/canvas/components/Guardrails/guardrail-validator-form.tsx | Implements validator parameter form using MetadataForm + guardrail-owned custom fields. |
| packages/apollo-react/src/canvas/components/Guardrails/guardrail-form-layout.tsx | Adds shared modal/inline layout wrapper for guardrail builder forms. |
| packages/apollo-react/src/canvas/components/Guardrails/guardrail-form-layout.test.tsx | Adds behavior + a11y tests for the shared form layout. |
| packages/apollo-react/src/canvas/components/Guardrails/guardrail-form-layout.stories.tsx | Adds Storybook examples for the shared form layout modes. |
| packages/apollo-react/src/canvas/components/Guardrails/form-schema-builder.ts | Builds MetadataForm schemas for guardrail parameters + coercion helper. |
| packages/apollo-react/src/canvas/components/Guardrails/form-schema-builder.test.ts | Tests schema mapping/coercion rules for guardrail parameter definitions. |
| packages/apollo-react/src/canvas/components/Guardrails/definitions-wire.ts | Adds hand-written wire types for the definitions endpoint payload. |
| packages/apollo-react/src/canvas/components/Guardrails/definitions-parse.ts | Adds zod validation + non-throwing parse result and issue reporting. |
| packages/apollo-react/src/canvas/components/Guardrails/definitions-parse.test.ts | Tests parsing guarantees + zod boundary constraints. |
| packages/apollo-react/src/canvas/components/Guardrails/definitions-parity.test.ts | Ensures canonical English matches Flow/Agents baselines and declares divergences. |
| packages/apollo-react/src/canvas/components/Guardrails/definitions-enrich.ts | Adds pure enrichment (copy resolution + parameter shaping + folder metadata helper). |
| packages/apollo-react/src/canvas/components/Guardrails/definitions-copy.test.ts | Tests copy table, message id conventions, and catalog parity. |
| packages/apollo-react/src/canvas/components/Guardrails/components/parameter-label.tsx | Shared parameter label renderer (required marker + info tooltip). |
| packages/apollo-react/src/canvas/components/Guardrails/components/mixed-scopes-banner.tsx | Adds banner for mixed-scope guardrails with “save as new” hint. |
| packages/apollo-react/src/canvas/components/Guardrails/components/mixed-scopes-banner.test.tsx | Tests mixed-scopes banner rendering + a11y. |
| packages/apollo-react/src/canvas/components/Guardrails/components/map-enum-field.tsx | Adds map-enum editor bound to sibling enum-list selection via useWatch. |
| packages/apollo-react/src/canvas/components/Guardrails/components/guardrail-status-banner.tsx | Adds status banners for disabled/unauthorized/feature-disabled definitions. |
| packages/apollo-react/src/canvas/components/Guardrails/components/guardrail-status-banner.test.tsx | Tests status banner roles and a11y. |
| packages/apollo-react/src/canvas/components/Guardrails/components/guardrail-scope-selector.tsx | Adds scope/tool targeting selector using chips and self-healing behavior. |
| packages/apollo-react/src/canvas/components/Guardrails/components/guardrail-scope-selector.test.tsx | Tests selector behavior, targeting semantics, and a11y. |
| packages/apollo-react/src/canvas/components/Guardrails/components/guardrail-chip.tsx | Adds chip toggle component (CVA variants) for scopes/options. |
| packages/apollo-react/src/canvas/components/Guardrails/components/guardrail-chip.test.tsx | Tests chip pressed state, interactions, and a11y. |
| packages/apollo-react/src/canvas/components/Guardrails/components/guardrail-action-section.tsx | Adds action configuration section (log/block/filter/escalate). |
| packages/apollo-react/src/canvas/components/Guardrails/components/guardrail-action-section.test.tsx | Tests action section branching + a11y. |
| packages/apollo-react/src/canvas/components/Guardrails/components/field-shell.tsx | Adds non-input “field shell” container with error border option. |
| packages/apollo-react/src/canvas/components/Guardrails/components/field-shell.test.tsx | Tests field shell invalid styling toggle. |
| packages/apollo-react/src/canvas/components/Guardrails/components/enum-list-chips-field.tsx | Adds chip-based enum-list editor for small option sets. |
| packages/apollo-react/src/canvas/components/Guardrails/builder-utils.ts | Adds builder helpers for defaults and required-field validation. |
| packages/apollo-react/src/canvas/components/Guardrails/builder-utils.test.ts | Tests builder utils behaviors and edge cases. |
| packages/apollo-react/src/canvas/components/Guardrails/builder-types.ts | Adds public structural types for persisted guardrail values and builder slots. |
| packages/apollo-react/src/canvas/components/Guardrails/fixtures/host-copy-baselines.ts | Adds transcribed Flow/Agents English baselines for copy parity tests. |
| packages/apollo-react/src/canvas/components/Guardrails/fixtures/definitions-wire.fixtures.ts | Adds realistic wire fixtures for parsing/enrichment/copy tests. |
| packages/apollo-react/package.json | Exposes ./canvas/guardrails subpath and adds deps (class-variance-authority, jest-axe, @types/jest-axe). |
Review details
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
- Files reviewed: 81/82 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const [rowIds, setRowIds] = useState<string[]>(() => items.map(() => crypto.randomUUID())); | ||
| const [prevLength, setPrevLength] = useState(items.length); | ||
| if (prevLength !== items.length) { | ||
| setPrevLength(items.length); | ||
| setRowIds((prev) => | ||
| prev.length < items.length | ||
| ? [ | ||
| ...prev, | ||
| ...Array.from({ length: items.length - prev.length }, () => crypto.randomUUID()), | ||
| ] | ||
| : prev.slice(0, items.length) | ||
| ); | ||
| } |
📊 Coverage + size by packagePer-package coverage and bundle size on this PR. New-line coverage = of the source lines this PR adds or changes, the % hit by tests.
"Coverage" is each package's own |
Storybook visual diffBaseline is the deployed main Storybook, so changes merged to main after this branch was last updated can also appear here. Logs Updated (PT): Sep 16, 2026, 07:18:28 AM |
2ff0c35 to
c9d8f20
Compare
c9d8f20 to
d658731
Compare
…AL-574] Review of #1139 against the #1107/#1138 threads (plan/review-2026-09-11 §3.2). - Type the fetch mocks as `vi.fn<typeof fetch>`: `vi.fn(async () => ...)` infers a zero-parameter mock, so every `mock.calls[i]?.[1]` assertion was a TS2493/TS2339 under the repo's strict config. CI cannot see it (tests are excluded from `tsc` and biome does not typecheck), so it is checked with a throwaway tsconfig. - `loading` starts `true` when the hook is about to fetch, so a host rendering `loading ? <Spinner/> : <Empty/>` no longer flashes the empty state on first paint. - `refetch` is a no-op while the hook is disabled. It used to issue a real request whose result `parsed` then discarded in favour of `options.definitions`. - JSDoc and README: `options.definitions` is compared by identity (pass a stable reference), a failed request keeps the previous results, and the zod boundary is pinned by a source-level check plus two tests, not by shipped runtime assertions. - Name the map-enum `0..1` step `0.1` default as a product assumption and pin what keeps it safe: after the rebase onto `d658731b`, `min`/`max` are enforced through `validation` in `onChange` mode, but both that path and `getOutOfRangeParameterIds` are number-only, so a synthesized map-enum bound cannot reject a threshold map whose real range is different (harmful content is 0..6). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
64e1ff4 to
6c9b54a
Compare
Integration branch only: it exists so a host can pin one preview package carrying every open apollo stream. Not for merging into main. Rebuilt on 2026-09-11 after the whole stack moved onto #1138's current head (`d658731b`) and picked up a first pass of review fixes on #1139 and #1140. Reset to `feat/apollo-react-guardrail-list` and re-merged `feat/apollo-react-guardrail-palette` (#1147, AL-576). The chip files both branches carry merged clean, being byte-identical again. Of the 17 conflicts, `i18n.ts`, `i18n.test.ts` and the 13 locale catalogs are unchanged on both sides since the previous merge (`8856e4e0`), so its resolution was reused verbatim. `index.ts` is the union of both barrels, biome-sorted, and checked for a lost export. The README was rebuilt from `8856e4e0`'s merged copy with the three deltas since then reapplied (the new base's, #1139's review fixes, #1140's review fixes), all cleanly, so the sections still run along the data flow: definitions layer, list, palette, builder. Verified after the merge: Guardrails suite 453 passing (26 files), tsc and biome clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…AL-574] Review of #1139 against the #1107/#1138 threads (plan/review-2026-09-11 §3.2). - Type the fetch mocks as `vi.fn<typeof fetch>`: `vi.fn(async () => ...)` infers a zero-parameter mock, so every `mock.calls[i]?.[1]` assertion was a TS2493/TS2339 under the repo's strict config. CI cannot see it (tests are excluded from `tsc` and biome does not typecheck), so it is checked with a throwaway tsconfig. - `loading` starts `true` when the hook is about to fetch, so a host rendering `loading ? <Spinner/> : <Empty/>` no longer flashes the empty state on first paint. - `refetch` is a no-op while the hook is disabled. It used to issue a real request whose result `parsed` then discarded in favour of `options.definitions`. - JSDoc and README: `options.definitions` is compared by identity (pass a stable reference), a failed request keeps the previous results, and the zod boundary is pinned by a source-level check plus two tests, not by shipped runtime assertions. - Name the map-enum `0..1` step `0.1` default as a product assumption and pin what keeps it safe: after the rebase onto `d658731b`, `min`/`max` are enforced through `validation` in `onChange` mode, but both that path and `getOutOfRangeParameterIds` are number-only, so a synthesized map-enum bound cannot reject a threshold map whose real range is different (harmful content is 0..6). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
6c9b54a to
74a4be4
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate findings remain in enrichment, parsing, and hook request/state handling.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (5)
packages/apollo-react/src/canvas/components/Guardrails/definitions-enrich.ts:174
wire.validatoris untrusted and the parser accepts names such asconstructorortoString. Indexing the normalRecordcan therefore return an inherited function instead ofundefined; when a parameter has no wire label,curated?.paramLabels[param.id]then dereferencesparamLabelson that function and enrichment throws, taking down the panel. Restrict the lookup to own properties (or use a null-prototype copy table) before enriching uncurated validators.
const isByo = isByoGuardrailDefinition(wire);
// Zero curated copy at every level for BYO: the validator id is the customer's, and a
// collision with a UiPath id must not borrow UiPath's wording.
const curated = isByo ? undefined : copy[wire.validator];
packages/apollo-react/src/canvas/components/Guardrails/definitions-enrich.ts:121
param.idalso indexesparamTooltipswithout an own-property check. For an id likeconstructor, this resolves the inherited function on the plain record and passes it asdefinition.tooltip;ParameterLabelthen hands that non-string value toInfoTooltipinstead of treating the parameter as uncurated. Use an own-property check for this lookup as well.
const tooltip = param.description ?? curated?.paramTooltips?.[param.id];
packages/apollo-react/src/canvas/components/Guardrails/definitions-enrich.ts:130
- The parser accepts empty strings in
optionLabels, and this merge gives those wire values precedence over the curated labels. The form later rendersdef.optionLabels?.[opt] ?? opt, so an empty backend label produces a visibly blank option instead of falling back to the curated/raw value. Normalize or discard blank wire labels before merging them.
param.type === 'enum' || param.type === 'enum-list' ? param.optionLabels : undefined;
if (curatedOptionLabels !== undefined || wireOptionLabels !== undefined) {
// Wire last: a manifest may relabel a subset without restating the rest. Options nobody
// labelled stay absent; the editors fall back to the raw wire value.
definition.optionLabels = { ...curatedOptionLabels, ...wireOptionLabels };
packages/apollo-react/src/canvas/components/Guardrails/use-guardrail-definitions.ts:114
- The hook claims to compare the request context by content, but serializing
headersdirectly makes semantically identical records with different insertion order produce different keys. A host that rebuilds the same headers in a different order on rerender will abort/refetch unnecessarily, potentially reintroducing the non-settling request loop this hook is meant to avoid. Canonicalize the header entries before serializing the request key.
headers: ctx.headers,
packages/apollo-react/src/canvas/components/Guardrails/use-guardrail-definitions.ts:206
- When the hook is disabled, this only makes the previous
settledresult unreachable whilectxisnull; it does not clear it. Re-enabling the same context makessettled.key === requestKeyagain, so the old tenant's definitions can reappear before the new request completes and are even retained if that request fails, contrary to the documented “Disabling the hook does clear the fetched state” behavior. Reset the settled stamp on the disable path so re-enabling always starts without stale data.
// Only the in-flight flag needs clearing; the results are already unreachable, since
// their stamp cannot match a disabled hook's request.
abortRef.current?.abort();
setInFlight(false);
- Files reviewed: 23/23 changed files
- Comments generated: 2
- Review effort level: Lite
| label: | ||
| param.displayName ?? curated?.paramLabels[param.id] ?? humanizeGuardrailParameterId(param.id), |
| input.forEach((entry: unknown, index: number) => { | ||
| // A hostile payload must not be able to take a host panel down, so even an unexpected | ||
| // throw degrades to a dropped definition. `readValidator` is inside the try as well: it | ||
| // reads `entry.validator` a second time, and a throwing getter there would escape a |
77d375f to
017e078
Compare
Add `GuardrailList`, the applied-guardrails list both products render under a tool or an agent: rows with their lifecycle and status chips, BYO notices, per-row edit and remove, drag reordering, and the empty state. The host filters and the list renders. Items arrive already resolved against their definitions, every callback is an intent, and `renderItemActions`, `statusBanner` and `addSlot` are slots rather than product branches, so no telemetry, flag or product type crosses the boundary. `GuardrailListItem` is a structural mirror of what both products already hold. Reordering is dnd-kit with `verticalListSortingStrategy`, restricted to the vertical axis and the parent element (`@dnd-kit/modifiers`, the one new dependency). The row transform is `CSS.Translate`, never `CSS.Transform`: `useSortable` derives its layout transform from the row's before and after rects, so with variable row heights (a description line, a BYO notice) `CSS.Transform` emits a `scaleY` that visibly squashes or stretches the row's text mid-animation. The strategy never produces a scale of its own, so dropping it costs nothing. Verified against the installed `@dnd-kit/sortable` source, which the published docs do not cover. Rows highlight on hover unconditionally, like wind's `TableRow`: a row is a hover target whether or not clicking it opens the editor. `rowActivatesEdit` restores the legacy click-to-edit behaviour, and only then does the row body carry `cursor-pointer` and its own focus ring, since only then is it a control. The tint is `accent`, Apollo's hover surface (`--accent: var(--surface-hover)`), not `muted`, which is `surface-overlay`, the panel the list usually sits on and therefore invisible against it. The family-wide rule is in the README. Seventeen `guardrails.list.*` lingui ids. Twelve take their English from Flow's canvas catalog, so the wording is what the product already ships; the other five have no host equivalent and are newly written here (`edit-row`, `status-feature-disabled`, `status-disabled`, `status-unavailable`, `administration-governance`), so they are the ones needing a real loc pass rather than a lookup. English only, like every other string in this package: `chore(l10n): sync from Localization` owns the other thirteen catalogs. The i18n test uses the family's shared catalog scans (`__fixtures__/catalog-coverage`) rather than its own locale list and reader. The "Preview" lifecycle chip takes the family's `info` tone, added to `GuardrailStatusChip` on #1139 for this and the palette: blue is what both products already give that chip (Flow's `InfoBadge tone="preview"`, Agents' `PreviewChip` on `semantic.colorInfoBackground`), where this row shipped it neutral grey. `GuardrailStatusBanner` and `MixedScopesBanner` become public so a host can compose the `statusBanner` slot, and each gains a story. `MixedScopesBanner` also gains the `mt-0` the title-less alert fix should have given it: wind's `AlertDescription` is unconditionally `mt-1`, which assumes an `AlertTitle` above it, so a title-less alert renders its text 4px below its icon. The same hunk is on apollo-ui#1147 and apollo-ui#1161 so the three merge without a conflict; the real fix is one line in wind's `alertVariants`. `__fixtures__/**` is excluded from the rslib build alongside `src/test/**`: fixtures are data for the suites, not API, and they reach for devDependencies. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Add `GuardrailPalette`, the picker both products open from their guardrails list: grouped definitions, an optional create-custom entry, and the loading, failed and empty states. The host filters and the palette offers. Definitions arrive pre-filtered (flags, entitlements, `FeatureDisabled`/`Disabled`, Tool scope), both callbacks are intents, and no telemetry, flag or product type crosses the boundary. Definitions are generic over the eight fields the palette reads, so `EnrichedGuardrailDefinition` and a product's own type both go in and come back out of `onSelectOotb` unchanged. Grouping and entry identity are re-derived from what both products already ship: one unheaded group in payload order without BYO definitions, otherwise a group per BYO folder or connector plus a trailing UiPath group, and BYO entries keyed by validator name and connection id. An `Unauthorised` definition is offered, chipped and not choosable (`aria-disabled`, so it stays reachable), which is Flow's behaviour; Agents lets it through to a builder that then refuses to save. Two chips take harvested tones rather than neutral grey: the bring-your-own connector chip is `success`, the green both products already give it, and the "Preview" lifecycle chip is `info`, the blue both products give that one (added to `GuardrailStatusChip` on #1139 for this and the list). Entries are real buttons on wind's interactive-item idiom, not a `div` with `role="listitem"` and its own key handler: hover is `bg-accent` gated on the enabled branch, disabled is `opacity-50`, and the focus ring matches `Button`. The palette is a single tab stop with roving focus, where Arrow/Home/End cross group boundaries and `aria-disabled` entries are included so the `Unauthorised` chip stays reachable. The focused entry is located from the keyboard event target rather than `document.activeElement`, which retargets to the shadow host inside Agents' shadow root and would make every arrow key a silent no-op there; a test renders into an open shadow root and arrows down. Groups name themselves with `aria-labelledby` pointing at the visible header rather than repeating it. Only the picker ships. Flow's dialog and inline overlay and Agents' sidebar takeover are host orchestration, shown in the stories rather than modelled in a wrapper. Nine `guardrails.palette.*` lingui ids, harvested from the two products' own catalogs so the English is what they already ship. English only, like every other string in this package: `chore(l10n): sync from Localization` owns the other thirteen catalogs. The i18n test uses the family's shared catalog scans (`__fixtures__/catalog-coverage`) rather than its own locale list and file reader. Three label resolvers now share one `mergeLabels` helper. Also carries the title-less alert fix the sibling branches each need: `AlertDescription` is unconditionally `mt-1`, which assumes an `AlertTitle` above it, so a title-less alert renders its text 4px below the icon. `mt-0` at the call site, the same hunk as apollo-ui#1140 and apollo-ui#1161 so the three merge without a conflict; the real fix belongs in apollo-wind. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Address catalog ownership documentation, prototype-safe lookups, and disabled-hook state reset.
Review details
Suppressed comments (4)
packages/apollo-react/src/canvas/components/Guardrails/definitions-copy.ts:29
- This says Lingui extraction will see the static calls, but this module uses
useSafeLinguidescriptors and the catalog-coverage tests explicitly document thatlingui extractdoes not feedsrc/canvas/locales. That is misleading for future additions and could lead to message IDs being omitted from the hand-authored English catalog; state that the catalog is maintained separately and checked by tests instead.
* and the runtime lingui path cannot drift, and `lingui extract` still sees static calls.
packages/apollo-react/src/canvas/components/Guardrails/definitions-enrich.ts:174
copyis a normal object, so an otherwise valid uncurated wire validator such astoString,constructor, or__proto__resolves an inherited value here.toParameterDefinitionthen dereferencescurated?.paramLabels[param.id]on that function/prototype and can throw during enrichment, defeating the parser's per-definition failure isolation. Restrict the lookup to own properties so unknown validators use the documented fallback path.
const curated = isByo ? undefined : copy[wire.validator];
packages/apollo-react/src/canvas/components/Guardrails/definitions-enrich.ts:116
- The parser accepts any non-blank parameter id, but these normal-object lookups treat reserved ids such as
__proto__andtoStringas inherited labels/tooltips rather than missing entries. That can put an object or function into the display fields and make rendering fail instead of falling back to the humanized id. Use own-property checks forparamLabels,paramTooltips, andoptionLabels(or store these tables with null prototypes).
label:
param.displayName ?? curated?.paramLabels[param.id] ?? humanizeGuardrailParameterId(param.id),
packages/apollo-react/src/canvas/components/Guardrails/use-guardrail-definitions.ts:206
- This branch only makes the previous
settledresult unreachable while disabled; it does not clear it. If the hook is disabled and then re-enabled with the same request context,settled.keymatches on the first render, so the old payload is shown withloading: falsebefore the new effect runs, and a failed refetch can keep that stale payload. Reset the settled stamp here as the README's “Disabling the hook does clear the fetched state” contract requires.
// Only the in-flight flag needs clearing; the results are already unreachable, since
// their stamp cannot match a disabled hook's request.
abortRef.current?.abort();
setInFlight(false);
- Files reviewed: 23/23 changed files
- Comments generated: 0 new
- Review effort level: Lite
Add `GuardrailList`, the applied-guardrails list both products render under a tool or an agent: rows with their lifecycle and status chips, BYO notices, per-row edit and remove, drag reordering, and the empty state. The host filters and the list renders. Items arrive already resolved against their definitions, every callback is an intent, and `renderItemActions`, `statusBanner` and `addSlot` are slots rather than product branches, so no telemetry, flag or product type crosses the boundary. `GuardrailListItem` is a structural mirror of what both products already hold. Reordering is dnd-kit with `verticalListSortingStrategy`, restricted to the vertical axis and the parent element (`@dnd-kit/modifiers`, the one new dependency). The row transform is `CSS.Translate`, never `CSS.Transform`: `useSortable` derives its layout transform from the row's before and after rects, so with variable row heights (a description line, a BYO notice) `CSS.Transform` emits a `scaleY` that visibly squashes or stretches the row's text mid-animation. The strategy never produces a scale of its own, so dropping it costs nothing. Verified against the installed `@dnd-kit/sortable` source, which the published docs do not cover. Rows highlight on hover unconditionally, like wind's `TableRow`: a row is a hover target whether or not clicking it opens the editor. `rowActivatesEdit` restores the legacy click-to-edit behaviour, and only then does the row body carry `cursor-pointer` and its own focus ring, since only then is it a control. The tint is `accent`, Apollo's hover surface (`--accent: var(--surface-hover)`), not `muted`, which is `surface-overlay`, the panel the list usually sits on and therefore invisible against it. The family-wide rule is in the README. Because the row paints that tint, the drag handle carries no negative margin: `p-1` gives the row 4px, which is exactly the reach of the handle's `ring-offset-2` focus ring, so pulling the handle out to align its glyph with the row's leading edge put its hover surface and its ring outside the tinted box. The glyph sits an icon button's worth of padding inside the edge instead. Seventeen `guardrails.list.*` lingui ids. Twelve take their English from Flow's canvas catalog, so the wording is what the product already ships; the other five have no host equivalent and are newly written here (`edit-row`, `status-feature-disabled`, `status-disabled`, `status-unavailable`, `administration-governance`), so they are the ones needing a real loc pass rather than a lookup. English only, like every other string in this package: `chore(l10n): sync from Localization` owns the other thirteen catalogs. The i18n test uses the family's shared catalog scans (`__fixtures__/catalog-coverage`) rather than its own locale list and reader. The "Preview" lifecycle chip takes the family's `info` tone, added to `GuardrailStatusChip` on #1139 for this and the palette: blue is what both products already give that chip (Flow's `InfoBadge tone="preview"`, Agents' `PreviewChip` on `semantic.colorInfoBackground`), where this row shipped it neutral grey. `GuardrailStatusBanner` and `MixedScopesBanner` become public so a host can compose the `statusBanner` slot, and each gains a story. `MixedScopesBanner` also gains the `mt-0` the title-less alert fix should have given it: wind's `AlertDescription` is unconditionally `mt-1`, which assumes an `AlertTitle` above it, so a title-less alert renders its text 4px below its icon. The same hunk is on apollo-ui#1147 and apollo-ui#1161 so the three merge without a conflict; the real fix is one line in wind's `alertVariants`. `__fixtures__/**` is excluded from the rslib build alongside `src/test/**`: fixtures are data for the suites, not API, and they reach for devDependencies. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Add `GuardrailPalette`, the picker both products open from their guardrails list: grouped definitions, an optional create-custom entry, and the loading, failed and empty states. The host filters and the palette offers. Definitions arrive pre-filtered (flags, entitlements, `FeatureDisabled`/`Disabled`, Tool scope), both callbacks are intents, and no telemetry, flag or product type crosses the boundary. Definitions are generic over the eight fields the palette reads, so `EnrichedGuardrailDefinition` and a product's own type both go in and come back out of `onSelectOotb` unchanged. Grouping and entry identity are re-derived from what both products already ship: one unheaded group in payload order without BYO definitions, otherwise a group per BYO folder or connector plus a trailing UiPath group, and BYO entries keyed by validator name and connection id. An `Unauthorised` definition is offered, chipped and not choosable (`aria-disabled`, so it stays reachable), which is Flow's behaviour; Agents lets it through to a builder that then refuses to save. Two chips take harvested tones rather than neutral grey: the bring-your-own connector chip is `success`, the green both products already give it, and the "Preview" lifecycle chip is `info`, the blue both products give that one (added to `GuardrailStatusChip` on #1139 for this and the list). Entries are real buttons on wind's interactive-item idiom, not a `div` with `role="listitem"` and its own key handler: hover is `bg-accent` gated on the enabled branch, disabled is `opacity-50`, and the focus ring matches `Button`. The palette is a single tab stop with roving focus, where Arrow/Home/End cross group boundaries and `aria-disabled` entries are included so the `Unauthorised` chip stays reachable. The focused entry is located from the keyboard event target rather than `document.activeElement`, which retargets to the shadow host inside Agents' shadow root and would make every arrow key a silent no-op there; a test renders into an open shadow root and arrows down. Groups name themselves with `aria-labelledby` pointing at the visible header rather than repeating it. Only the picker ships. Flow's dialog and inline overlay and Agents' sidebar takeover are host orchestration, shown in the stories rather than modelled in a wrapper. Nine `guardrails.palette.*` lingui ids, harvested from the two products' own catalogs so the English is what they already ship. English only, like every other string in this package: `chore(l10n): sync from Localization` owns the other thirteen catalogs. The i18n test uses the family's shared catalog scans (`__fixtures__/catalog-coverage`) rather than its own locale list and file reader. Three label resolvers now share one `mergeLabels` helper. Also carries the title-less alert fix the sibling branches each need: `AlertDescription` is unconditionally `mt-1`, which assumes an `AlertTitle` above it, so a title-less alert renders its text 4px below the icon. `mt-0` at the call site, the same hunk as apollo-ui#1140 and apollo-ui#1161 so the three merge without a conflict; the real fix belongs in apollo-wind. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
017e078 to
9a44c53
Compare
Add `GuardrailList`, the applied-guardrails list both products render under a tool or an agent: rows with their lifecycle and status chips, BYO notices, per-row edit and remove, drag reordering, and the empty state. The host filters and the list renders. Items arrive already resolved against their definitions, every callback is an intent, and `renderItemActions`, `statusBanner` and `addSlot` are slots rather than product branches, so no telemetry, flag or product type crosses the boundary. `GuardrailListItem` is a structural mirror of what both products already hold. Reordering is dnd-kit with `verticalListSortingStrategy`, restricted to the vertical axis and the parent element (`@dnd-kit/modifiers`, the one new dependency). The row transform is `CSS.Translate`, never `CSS.Transform`: `useSortable` derives its layout transform from the row's before and after rects, so with variable row heights (a description line, a BYO notice) `CSS.Transform` emits a `scaleY` that visibly squashes or stretches the row's text mid-animation. The strategy never produces a scale of its own, so dropping it costs nothing. Verified against the installed `@dnd-kit/sortable` source, which the published docs do not cover. Rows highlight on hover unconditionally, like wind's `TableRow`: a row is a hover target whether or not clicking it opens the editor. `rowActivatesEdit` restores the legacy click-to-edit behaviour, and only then does the row body carry `cursor-pointer` and its own focus ring, since only then is it a control. The tint is `accent`, Apollo's hover surface (`--accent: var(--surface-hover)`), not `muted`, which is `surface-overlay`, the panel the list usually sits on and therefore invisible against it. The family-wide rule is in the README. Because the row paints that tint, the drag handle carries no negative margin: `p-1` gives the row 4px, which is exactly the reach of the handle's `ring-offset-2` focus ring, so pulling the handle out to align its glyph with the row's leading edge put its hover surface and its ring outside the tinted box. The glyph sits an icon button's worth of padding inside the edge instead. Seventeen `guardrails.list.*` lingui ids. Twelve take their English from Flow's canvas catalog, so the wording is what the product already ships; the other five have no host equivalent and are newly written here (`edit-row`, `status-feature-disabled`, `status-disabled`, `status-unavailable`, `administration-governance`), so they are the ones needing a real loc pass rather than a lookup. English only, like every other string in this package: `chore(l10n): sync from Localization` owns the other thirteen catalogs. The i18n test uses the family's shared catalog scans (`__fixtures__/catalog-coverage`) rather than its own locale list and reader, and the stories carry no Japanese example: with the translations gone it would render English and claim otherwise. The "Preview" lifecycle chip takes the family's `info` tone, added to `GuardrailStatusChip` on #1139 for this and the palette: blue is what both products already give that chip (Flow's `InfoBadge tone="preview"`, Agents' `PreviewChip` on `semantic.colorInfoBackground`), where this row shipped it neutral grey. `GuardrailStatusBanner` and `MixedScopesBanner` become public so a host can compose the `statusBanner` slot, and each gains a story. `MixedScopesBanner` also gains the `mt-0` the title-less alert fix should have given it: wind's `AlertDescription` is unconditionally `mt-1`, which assumes an `AlertTitle` above it, so a title-less alert renders its text 4px below its icon. The same hunk is on apollo-ui#1147 and apollo-ui#1161 so the three merge without a conflict; the real fix is one line in wind's `alertVariants`. `__fixtures__/**` is excluded from the rslib build alongside `src/test/**`: fixtures are data for the suites, not API, and they reach for devDependencies. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate findings remain in parser traversal, hook request-key and override-state handling, and catalog documentation.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (5)
packages/apollo-react/src/canvas/components/Guardrails/components/guardrail-status-chip.stories.tsx:13
- This story calls the governance axis “origin”, but the component's own API documentation names it “administration” (line 24), while “origin” is reserved for the separate BYO-versus-managed distinction. Keeping the terminology consistent avoids documenting the two different axes as if they were the same.
origin in the centralized section. It is a span carrying the wind badge classes, not a
packages/apollo-react/src/canvas/components/Guardrails/definitions-copy.ts:29
_here is supplied byuseSafeLingui, not a Lingui macro, and the README below explicitly documents thatlingui extractdoes not scan canvas calls. This claim is therefore misleading and could make maintainers expect new definition IDs to be generated automatically; describe the catalog as hand-authored instead.
* and the runtime lingui path cannot drift, and `lingui extract` still sees static calls.
packages/apollo-react/src/canvas/components/Guardrails/definitions-parse.ts:222
- The parser's
never throwscontract is still breakable before the per-entrytry: this calls the payload's ownforEach. A public caller (including a structured-clonedpostMessagearray) can provide an array with an own non-callable or throwingforEach, which makes this function throw instead of returning aninputError/invalidresult. Iterate by index (usingcontinuefor the successful entry) or otherwise guard the array traversal so the top-level contract holds for every array input.
input.forEach((entry: unknown, index: number) => {
packages/apollo-react/src/canvas/components/Guardrails/use-guardrail-definitions.ts:124
- Because
definitionsis an optionalunknown, SWR/React Query data is commonlyundefinedduring its initial load. This condition treats that state as “no override” and starts the hook's own request, despite the documentedoptions.definitionsseam saying that supplying the option disables fetching. Track property presence separately (or add an explicit external-loading state) so a host cache does not trigger a second request while it is unresolved.
const enabled = provided === undefined && request !== null;
packages/apollo-react/src/canvas/components/Guardrails/use-guardrail-definitions.ts:114
- The request key is described as content-based, but
JSON.stringifypreserves object insertion order forheaders. Two equivalent header maps built in different orders produce different keys, so the effect aborts and refetches unnecessarily; if a host alternates that order across renders, this can recreate the refetch loop this hook is intended to avoid. Canonicalize the header entries before serializing them.
headers: ctx.headers,
- Files reviewed: 23/23 changed files
- Comments generated: 1
- Review effort level: Lite
| const isCurrent = settled.key === requestKey; | ||
| const fetched = isCurrent ? settled.result : EMPTY_RESULT; | ||
| const error = isCurrent ? settled.error : null; |
Add `GuardrailPalette`, the picker both products open from their guardrails list: grouped definitions, an optional create-custom entry, and the loading, failed and empty states. The host filters and the palette offers. Definitions arrive pre-filtered (flags, entitlements, `FeatureDisabled`/`Disabled`, Tool scope), both callbacks are intents, and no telemetry, flag or product type crosses the boundary. Definitions are generic over the eight fields the palette reads, so `EnrichedGuardrailDefinition` and a product's own type both go in and come back out of `onSelectOotb` unchanged. Grouping and entry identity are re-derived from what both products already ship: one unheaded group in payload order without BYO definitions, otherwise a group per BYO folder or connector plus a trailing UiPath group, and BYO entries keyed by validator name and connection id. An `Unauthorised` definition is offered, chipped and not choosable (`aria-disabled`, so it stays reachable), which is Flow's behaviour; Agents lets it through to a builder that then refuses to save. Two chips take harvested tones rather than neutral grey: the bring-your-own connector chip is `success`, the green both products already give it, and the "Preview" lifecycle chip is `info`, the blue both products give that one (added to `GuardrailStatusChip` on #1139 for this and the list). Entries are real buttons on wind's interactive-item idiom, not a `div` with `role="listitem"` and its own key handler: hover is `bg-accent` gated on the enabled branch, disabled is `opacity-50`, and the focus ring matches `Button`. The palette is a single tab stop with roving focus, where Arrow/Home/End cross group boundaries and `aria-disabled` entries are included so the `Unauthorised` chip stays reachable. The focused entry is located from the keyboard event target rather than `document.activeElement`, which retargets to the shadow host inside Agents' shadow root and would make every arrow key a silent no-op there; a test renders into an open shadow root and arrows down. Groups name themselves with `aria-labelledby` pointing at the visible header rather than repeating it. Only the picker ships. Flow's dialog and inline overlay and Agents' sidebar takeover are host orchestration, shown in the stories rather than modelled in a wrapper. Nine `guardrails.palette.*` lingui ids, harvested from the two products' own catalogs so the English is what they already ship. English only, like every other string in this package: `chore(l10n): sync from Localization` owns the other thirteen catalogs. The i18n test uses the family's shared catalog scans (`__fixtures__/catalog-coverage`) rather than its own locale list and file reader. Three label resolvers now share one `mergeLabels` helper. Also carries the title-less alert fix the sibling branches each need: `AlertDescription` is unconditionally `mt-1`, which assumes an `AlertTitle` above it, so a title-less alert renders its text 4px below the icon. `mt-0` at the call site, the same hunk as apollo-ui#1140 and apollo-ui#1161 so the three merge without a conflict; the real fix belongs in apollo-wind. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
9a44c53 to
9086203
Compare
Add `GuardrailList`, the applied-guardrails list both products render under a tool or an agent: rows with their lifecycle and status chips, BYO notices, per-row edit and remove, drag reordering, and the empty state. The host filters and the list renders. Items arrive already resolved against their definitions, every callback is an intent, and `renderItemActions`, `statusBanner` and `addSlot` are slots rather than product branches, so no telemetry, flag or product type crosses the boundary. `GuardrailListItem` is a structural mirror of what both products already hold. Reordering is dnd-kit with `verticalListSortingStrategy`, restricted to the vertical axis and the parent element (`@dnd-kit/modifiers`, the one new dependency). The row transform is `CSS.Translate`, never `CSS.Transform`: `useSortable` derives its layout transform from the row's before and after rects, so with variable row heights (a description line, a BYO notice) `CSS.Transform` emits a `scaleY` that visibly squashes or stretches the row's text mid-animation. The strategy never produces a scale of its own, so dropping it costs nothing. Verified against the installed `@dnd-kit/sortable` source, which the published docs do not cover. Rows highlight on hover unconditionally, like wind's `TableRow`: a row is a hover target whether or not clicking it opens the editor. `rowActivatesEdit` restores the legacy click-to-edit behaviour, and only then does the row body carry `cursor-pointer` and its own focus ring, since only then is it a control. The tint is `accent`, Apollo's hover surface (`--accent: var(--surface-hover)`), not `muted`, which is `surface-overlay`, the panel the list usually sits on and therefore invisible against it. The family-wide rule is in the README. Because the row paints that tint, the drag handle carries no negative margin: `p-1` gives the row 4px, which is exactly the reach of the handle's `ring-offset-2` focus ring, so pulling the handle out to align its glyph with the row's leading edge put its hover surface and its ring outside the tinted box. The glyph sits an icon button's worth of padding inside the edge instead. Seventeen `guardrails.list.*` lingui ids. Twelve take their English from Flow's canvas catalog, so the wording is what the product already ships; the other five have no host equivalent and are newly written here (`edit-row`, `status-feature-disabled`, `status-disabled`, `status-unavailable`, `administration-governance`), so they are the ones needing a real loc pass rather than a lookup. English only, like every other string in this package: `chore(l10n): sync from Localization` owns the other thirteen catalogs. The i18n test uses the family's shared catalog scans (`__fixtures__/catalog-coverage`) rather than its own locale list and reader, and the stories carry no Japanese example: with the translations gone it would render English and claim otherwise. The "Preview" lifecycle chip takes the family's `info` tone, added to `GuardrailStatusChip` on #1139 for this and the palette: blue is what both products already give that chip (Flow's `InfoBadge tone="preview"`, Agents' `PreviewChip` on `semantic.colorInfoBackground`), where this row shipped it neutral grey. `GuardrailStatusBanner` and `MixedScopesBanner` become public so a host can compose the `statusBanner` slot, and each gains a story. `MixedScopesBanner` also gains the `mt-0` the title-less alert fix should have given it: wind's `AlertDescription` is unconditionally `mt-1`, which assumes an `AlertTitle` above it, so a title-less alert renders its text 4px below its icon. The same hunk is on apollo-ui#1147 and apollo-ui#1161 so the three merge without a conflict; the real fix is one line in wind's `alertVariants`. `__fixtures__/**` is excluded from the rslib build alongside `src/test/**`: fixtures are data for the suites, not API, and they reach for devDependencies. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Add `GuardrailPalette`, the picker both products open from their guardrails list: grouped definitions, an optional create-custom entry, and the loading, failed and empty states. The host filters and the palette offers. Definitions arrive pre-filtered (flags, entitlements, `FeatureDisabled`/`Disabled`, Tool scope), both callbacks are intents, and no telemetry, flag or product type crosses the boundary. Definitions are generic over the eight fields the palette reads, so `EnrichedGuardrailDefinition` and a product's own type both go in and come back out of `onSelectOotb` unchanged. Grouping and entry identity are re-derived from what both products already ship: one unheaded group in payload order without BYO definitions, otherwise a group per BYO folder or connector plus a trailing UiPath group, and BYO entries keyed by validator name and connection id. An `Unauthorised` definition is offered, chipped and not choosable (`aria-disabled`, so it stays reachable), which is Flow's behaviour; Agents lets it through to a builder that then refuses to save. Two chips take harvested tones rather than neutral grey: the bring-your-own connector chip is `success`, the green both products already give it, and the "Preview" lifecycle chip is `info`, the blue both products give that one (added to `GuardrailStatusChip` on #1139 for this and the list). Entries are real buttons on wind's interactive-item idiom, not a `div` with `role="listitem"` and its own key handler: hover is `bg-accent` gated on the enabled branch, disabled is `opacity-50`, and the focus ring matches `Button`. The palette is a single tab stop with roving focus, where Arrow/Home/End cross group boundaries and `aria-disabled` entries are included so the `Unauthorised` chip stays reachable. The focused entry is located from the keyboard event target rather than `document.activeElement`, which retargets to the shadow host inside Agents' shadow root and would make every arrow key a silent no-op there; a test renders into an open shadow root and arrows down. Groups name themselves with `aria-labelledby` pointing at the visible header rather than repeating it. Only the picker ships. Flow's dialog and inline overlay and Agents' sidebar takeover are host orchestration, shown in the stories rather than modelled in a wrapper. Nine `guardrails.palette.*` lingui ids, harvested from the two products' own catalogs so the English is what they already ship. English only, like every other string in this package: `chore(l10n): sync from Localization` owns the other thirteen catalogs. The i18n test uses the family's shared catalog scans (`__fixtures__/catalog-coverage`) rather than its own locale list and file reader. Three label resolvers now share one `mergeLabels` helper. Also carries the title-less alert fix the sibling branches each need: `AlertDescription` is unconditionally `mt-1`, which assumes an `AlertTitle` above it, so a title-less alert renders its text 4px below the icon. `mt-0` at the call site, the same hunk as apollo-ui#1140 and apollo-ui#1161 so the three merge without a conflict; the real fix belongs in apollo-wind. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical parser and enrichment findings, along with moderate hook and validation issues, block approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (6)
packages/apollo-react/src/canvas/components/Guardrails/definitions-enrich.ts:116
- Parameter ids are arbitrary wire strings, but this indexed lookup also traverses
paramLabels' prototype. A BYO or newly shipped parameter namedtoString/constructortherefore receives the inherited function as itslabelinstead of a string (and React can fail when rendering it), rather than using the humanized fallback. Guard the curated lookup withObject.hasOwn(and apply the same own-key handling to the tooltip/option-label lookups) before falling back.
label:
param.displayName ?? curated?.paramLabels[param.id] ?? humanizeGuardrailParameterId(param.id),
packages/apollo-react/src/canvas/components/Guardrails/definitions-parse.ts:46
- Empty option labels pass this schema, but enrichment treats a present wire label as authoritative and the form's
??fallback does not apply to''. A malformed manifest can therefore render a blank enum option instead of the raw or curated label. Normalize blank values away or reject them here; apply the same rule to the enum-list schema below.
optionLabels: z.record(z.string(), z.string()).optional(),
packages/apollo-react/src/canvas/components/Guardrails/definitions-parse.ts:56
- The enum-list schema has the same blank-label hole:
''/whitespace is accepted and later wins over curated labels, so the option renders without a visible label. Keep option-label validation consistent with the enum schema so malformed labels fall back or are reported rather than reaching the UI.
optionLabels: z.record(z.string(), z.string()).optional(),
packages/apollo-react/src/canvas/components/Guardrails/definitions-parse.ts:222
- This function promises never to throw, but the iteration dispatches through the untrusted array's
forEachproperty. An array with an ownforEachoverride or a throwing getter escapes the per-entrytrybefore a result is returned. Call the intrinsicArray.prototype.forEach(and keep the iteration boundary guarded) so malformed containers receive the same non-throwing treatment as malformed entries.
input.forEach((entry: unknown, index: number) => {
packages/apollo-react/src/canvas/components/Guardrails/use-guardrail-definitions.ts:206
- This disabled path aborts the request but leaves
settledstamped with the last successful request. If the hook is disabled and then re-enabled with the same context (or an inlinedefinitionsoverride is removed),isCurrentbecomes true again, so the old definitions reappear withloading: false; a subsequent failed request also preserves them as if it were a same-request refetch. Reset the settled stamp here so re-enabling starts from empty/loading state as documented.
abortRef.current?.abort();
setInFlight(false);
packages/apollo-react/src/canvas/components/Guardrails/use-guardrail-definitions.ts:137
- When a previous request has failed,
settled.keycan still equal the currentrequestKey. If the host then suppliesoptions.definitionswithout changingctx,enabledbecomes false but this expression still exposes that stale transport error alongside the valid supplied definitions. Scope the error to the active fetch path (for example,enabled && isCurrent) so the result does not report a failure for a request that is no longer being used.
const error = isCurrent ? settled.error : null;
- Files reviewed: 23/23 changed files
- Comments generated: 2
- Review effort level: Lite
| type: z.literal('map-enum'), | ||
| defaultValue: z.record(z.string(), z.number()), | ||
| required: z.boolean(), | ||
| keySource: nonBlankString, |
…ilDefinitions [AL-574] Turns the `GET /api/execution/guardrails/definitions` payload into the `GuardrailDefinition`s `GuardrailBuilder` renders. Flow and Agents each carry their own copy of this today, and the two have drifted. - `definitions-wire.ts` mirrors the payload by hand, admitting both products' nullability variants, and reuses `GuardrailScope`/`GuardrailDefinitionStatus` so wire and display cannot drift. - `definitions-parse.ts` validates with zod and never throws: a non-array sets `inputError`, one bad definition is dropped whole into `invalid`. Blank display strings are the single normalization, since one would beat curated copy and render an empty label; blank identifiers fail the entry instead of silently changing a definition's identity. zod stays private, pinned to the hand-written mirror by a bidirectional assignability check on the hot path plus a key-set test and a source-level import guard, so no schema type reaches the emitted `.d.ts`. - `definitions-copy.ts` carries the six built-in validators' display copy as 63 lingui messages in the shared canvas catalog, replacing Agents' `OOB_GUARDRAILS_I8N` and Flow's `buildValidatorDisplayInfo`. Ids use raw wire values, never a transcribed slug, which is how the two products ended up keying the same entity as `finNationalId` and `fiNationalId`. English only: the l10n sync owns the other catalogs, as it does for every other string here. - `definitions-enrich.ts` resolves copy onto validated wire definitions. Pure and React-free, so Flow's vsix bridge calls it directly. Curated wins at definition level, wire wins at parameter level, BYO takes no curated copy. - `use-guardrail-definitions.ts` composes the three. `options.definitions` skips the request entirely, so each product keeps its own transport. The context is compared by content rather than identity, unlike `useDiscoveryModels`, where an inline object refetches every render and never settles. Results carry the request key that produced them, so a tenant switch cannot keep serving the previous tenant's guardrails. The 17 places the two products' English differs are each declared with a reason in `definitions-parity.test.ts` and asserted against both products' transcribed copy, so the suite fails on an undeclared difference or a wording we invented. Also lands two things the leaf PRs were each carrying their own copy of, since #1140, #1147 and #1161 all branch from here. `GuardrailStatusChip` is a read-only pill for a guardrail row: deliberately not `GuardrailChip`, which wraps a Radix `Toggle` and would put fake buttons in the tab order, and a `<span>` composed from wind's `badgeVariants` rather than `Badge`, which renders a `<div>` that is invalid inside the palette entry's `<button>`. `GUARDRAIL_CHIP_GEOMETRY` is extracted so the interactive and read-only pills stay one system. Its tones are harvested, not invented: `success` is the green both products give a BYO origin or connector chip, `info` the blue both give a "Preview" lifecycle label (Flow `bg-blue-100 text-blue-700`, Agents `semantic.colorInfoBackground`), so neither product's colour changes when the leaves adopt it. The label truncates with the full text on the chip's `title`, because the pill is a fixed height and the text is the host's: a governance label or a connector name long enough to wrap rendered two lines and spilled out of its own background. The label sits in an inner span so it can truncate at all, since `text-overflow` does not reach the anonymous flex item bare text becomes inside `inline-flex`. `__fixtures__/catalog-coverage.ts` holds the catalog scans that stand in for `lingui extract`, which never sees `src/canvas` because it uses no macros. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
9086203 to
b4e90c4
Compare
Add `GuardrailList`, the applied-guardrails list both products render under a tool or an agent: rows with their lifecycle and status chips, BYO notices, per-row edit and remove, drag reordering, and the empty state. The host filters and the list renders. Items arrive already resolved against their definitions, every callback is an intent, and `renderItemActions`, `statusBanner` and `addSlot` are slots rather than product branches, so no telemetry, flag or product type crosses the boundary. `GuardrailListItem` is a structural mirror of what both products already hold. Reordering is dnd-kit with `verticalListSortingStrategy`, restricted to the vertical axis and the parent element (`@dnd-kit/modifiers`, the one new dependency). The row transform is `CSS.Translate`, never `CSS.Transform`: `useSortable` derives its layout transform from the row's before and after rects, so with variable row heights (a description line, a BYO notice) `CSS.Transform` emits a `scaleY` that visibly squashes or stretches the row's text mid-animation. The strategy never produces a scale of its own, so dropping it costs nothing. Verified against the installed `@dnd-kit/sortable` source, which the published docs do not cover. Rows highlight on hover unconditionally, like wind's `TableRow`: a row is a hover target whether or not clicking it opens the editor. `rowActivatesEdit` restores the legacy click-to-edit behaviour, and only then does the row body carry `cursor-pointer` and its own focus ring, since only then is it a control. The tint is `accent`, Apollo's hover surface (`--accent: var(--surface-hover)`), not `muted`, which is `surface-overlay`, the panel the list usually sits on and therefore invisible against it. The family-wide rule is in the README. Because the row paints that tint, the drag handle carries no negative margin: `p-1` gives the row 4px, which is exactly the reach of the handle's `ring-offset-2` focus ring, so pulling the handle out to align its glyph with the row's leading edge put its hover surface and its ring outside the tinted box. The glyph sits an icon button's worth of padding inside the edge instead. Seventeen `guardrails.list.*` lingui ids. Twelve take their English from Flow's canvas catalog, so the wording is what the product already ships; the other five have no host equivalent and are newly written here (`edit-row`, `status-feature-disabled`, `status-disabled`, `status-unavailable`, `administration-governance`), so they are the ones needing a real loc pass rather than a lookup. English only, like every other string in this package: `chore(l10n): sync from Localization` owns the other thirteen catalogs. The i18n test uses the family's shared catalog scans (`__fixtures__/catalog-coverage`) rather than its own locale list and reader, and the stories carry no Japanese example: with the translations gone it would render English and claim otherwise. The "Preview" lifecycle chip takes the family's `info` tone, added to `GuardrailStatusChip` on #1139 for this and the palette: blue is what both products already give that chip (Flow's `InfoBadge tone="preview"`, Agents' `PreviewChip` on `semantic.colorInfoBackground`), where this row shipped it neutral grey. `GuardrailStatusBanner` and `MixedScopesBanner` become public so a host can compose the `statusBanner` slot, and each gains a story. `MixedScopesBanner` also gains the `mt-0` the title-less alert fix should have given it: wind's `AlertDescription` is unconditionally `mt-1`, which assumes an `AlertTitle` above it, so a title-less alert renders its text 4px below its icon. The same hunk is on apollo-ui#1147 and apollo-ui#1161 so the three merge without a conflict; the real fix is one line in wind's `alertVariants`. `__fixtures__/**` is excluded from the rslib build alongside `src/test/**`: fixtures are data for the suites, not API, and they reach for devDependencies. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Add `GuardrailPalette`, the picker both products open from their guardrails list: grouped definitions, an optional create-custom entry, and the loading, failed and empty states. The host filters and the palette offers. Definitions arrive pre-filtered (flags, entitlements, `FeatureDisabled`/`Disabled`, Tool scope), both callbacks are intents, and no telemetry, flag or product type crosses the boundary. Definitions are generic over the eight fields the palette reads, so `EnrichedGuardrailDefinition` and a product's own type both go in and come back out of `onSelectOotb` unchanged. Grouping and entry identity are re-derived from what both products already ship: one unheaded group in payload order without BYO definitions, otherwise a group per BYO folder or connector plus a trailing UiPath group, and BYO entries keyed by validator name and connection id. An `Unauthorised` definition is offered, chipped and not choosable (`aria-disabled`, so it stays reachable), which is Flow's behaviour; Agents lets it through to a builder that then refuses to save. Two chips take harvested tones rather than neutral grey: the bring-your-own connector chip is `success`, the green both products already give it, and the "Preview" lifecycle chip is `info`, the blue both products give that one (added to `GuardrailStatusChip` on #1139 for this and the list). Entries are real buttons on wind's interactive-item idiom, not a `div` with `role="listitem"` and its own key handler: hover is `bg-accent` gated on the enabled branch, disabled is `opacity-50`, and the focus ring matches `Button`. The palette is a single tab stop with roving focus, where Arrow/Home/End cross group boundaries and `aria-disabled` entries are included so the `Unauthorised` chip stays reachable. The focused entry is located from the keyboard event target rather than `document.activeElement`, which retargets to the shadow host inside Agents' shadow root and would make every arrow key a silent no-op there; a test renders into an open shadow root and arrows down. Groups name themselves with `aria-labelledby` pointing at the visible header rather than repeating it. Only the picker ships. Flow's dialog and inline overlay and Agents' sidebar takeover are host orchestration, shown in the stories rather than modelled in a wrapper. Nine `guardrails.palette.*` lingui ids, harvested from the two products' own catalogs so the English is what they already ship. English only, like every other string in this package: `chore(l10n): sync from Localization` owns the other thirteen catalogs. The i18n test uses the family's shared catalog scans (`__fixtures__/catalog-coverage`) rather than its own locale list and file reader. Three label resolvers now share one `mergeLabels` helper. Also carries the title-less alert fix the sibling branches each need: `AlertDescription` is unconditionally `mt-1`, which assumes an `AlertTitle` above it, so a title-less alert renders its text 4px below the icon. `mt-0` at the call site, the same hunk as apollo-ui#1140 and apollo-ui#1161 so the three merge without a conflict; the real fix belongs in apollo-wind. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Three moderate findings remain, along with one terminology nit.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
packages/apollo-react/src/canvas/components/Guardrails/definitions-copy.ts:29
useSafeLinguiis a regular wrapper, not a Lingui macro, and the catalog tests explicitly document thatlingui extractdoes not feedsrc/canvas. Saying extraction still sees these calls is incorrect and could lead maintainers to stop updating the hand-authored catalogs.
* One builder holds every message, so the English source, the flat record hosts diff in CI
* and the runtime lingui path cannot drift, and `lingui extract` still sees static calls.
packages/apollo-react/src/canvas/components/Guardrails/definitions-enrich.ts:116
param.idis parsed from the payload and is only required to be non-blank. For a curated validator, an id such astoStringorconstructortherefore reads an inheritedObject.prototypefunction fromparamLabelsinstead of a string, and the same unsafe lookup is used for tooltips below; the builder then receives a non-string label. The parameter-key and option-key lookups should be own-property-only (or use null-prototype maps).
label:
param.displayName ?? curated?.paramLabels[param.id] ?? humanizeGuardrailParameterId(param.id),
packages/apollo-react/src/canvas/components/Guardrails/use-guardrail-definitions.ts:206
- This branch only makes the old
settledresult unreachable whilerequestKeyis empty; it does not clear it. If the hook is disabled and then re-enabled with the same context,isCurrentbecomes true before the new effect runs, so the stale definitions are rendered withloading: false, and a failed refetch can retain them indefinitely. Reset the settled stamp here and add a disable/re-enable regression test.
if (!enabled) {
// Only the in-flight flag needs clearing; the results are already unreachable, since
// their stamp cannot match a disabled hook's request.
abortRef.current?.abort();
setInFlight(false);
- Files reviewed: 23/23 changed files
- Comments generated: 1
- Review effort level: Lite
| Read-only status label for a guardrail row: definition status in the palette, governance | ||
| origin in the centralized section. It is a span carrying the wind badge classes, not a |
Builds the shared guardrail definitions layer AL-574 asks for: the seam between the
/api/execution/guardrails/definitionspayload and theGuardrailDefinitionsGuardrailBuilderalready renders. Flow and Agents each carry their own copy of this today, and the two have drifted.
What lands
definitions-wire.tsGuardrailDefinitionWire,GuardrailParameterDefinitionWiredefinitions-parse.tsparseGuardrailDefinitionsdefinitions-copy.tsGUARDRAIL_COPY_EN,GUARDRAIL_COPY_EN_MESSAGES,useGuardrailDefinitionCopydefinitions-enrich.tsenrichGuardrailDefinitions,isByoGuardrailDefinition,humanizeGuardrailParameterId,withGuardrailFolderMetadatause-guardrail-definitions.tsuseGuardrailDefinitionsAlso
GuardrailStatusChipand the shared catalog-scan fixture, which #1140/#1147/#1161 were eachcarrying their own copy of; a README section; exports from
Guardrails/index.ts(nopackage.jsonchange,
./canvas/guardrailsalready points there); and theguardrails.definitions.*catalog ids.The chip's tones are harvested, not invented. It carries
neutral,info,success,warninganderror, mapped onto wind'sbadgeVariants. The two non-obvious ones exist becauseboth products already ship those colours and adopting the chip must not change either:
InfoBadge tone="custom"(green)ByoGuardrailConnectorChip,palette.green[100]/[700]successbg-blue-100 text-blue-700,InfoBadge tone="preview"PreviewChip,semantic.colorInfoBackground/colorInfoTextinfo#1161 consumes the first, #1140 and #1147 the second.
Decisions worth checking
inputError; one bad definition is droppedwhole and reported in
invalid, which is what both products already do entry by entry. Transporterrors and data errors are separate channels: a malformed payload leaves
errornull.definitions-parse.tsand pinned to thehand-written mirror by a bidirectional assignability check on the hot path, plus a key-set test
and a source-level import guard. Verified absent from the emitted
.d.ts.EnrichedGuardrailDefinition extends GuardrailDefinition, so its output feeds the builderunmapped.
options.definitionsskips the request entirely — Agents keeps SWR, Flow studio and workbenchkeep react-query, the vsix keeps postMessage.
hiddenValidatorshides nothing by default and never hides a BYO definition. Which validatorsa product exposes is an entitlement decision, so it stays with the caller.
useDiscoveryModels. Keying offidentity means an inline context refetches on every render and, since every response sets state,
never settles; the hook test caught it at 17,640 calls.
useDiscoveryModelsstill has thatfootgun, worth a separate look.
map-enumbounds. feat(apollo-react): guardrails component family under canvas #1138'sgetOutOfRangeParameterIdsnow range-checks map rowsand hosts gate Save on it, so a bound this layer made up would reject a threshold on a scale the
backend never stated. Wire bounds pass through;
stepstays a hint, since nothing enforces it.Canonical copy moves onto lingui
The six built-in validators' display copy lives twice today, in Agents'
OOB_GUARDRAILS_I8NandFlow's
buildValidatorDisplayInfo. Here it is 63 lingui messages in the shared canvas catalog, soboth products get the same wording and the strings enter the real loc pipeline.
Ids use raw wire values (
USSocialSecurityNumber), never a transcribed slug. Transcribing isexactly how the two products ended up keying the same Finland entity as
finNationalIdandfiNationalId.English only, like every other string in this package:
chore(l10n): sync from Localizationowns the other thirteen catalogs and appends new ids every week or two. Until it runs,
useSafeLinguirenders the English default, so nothing is blank on screen.One thing worth passing on from building this. An earlier revision did ship the translations,
harvested from the host tables, and the harvest surfaced a bug in the source:
es-MXhasSexualandViolencetranslated onto each other in the harmful-content entity labels, so thetwo severity rows are labelled with each other's category. That is not in this diff any more, but
it is presumably still live in whichever host table it came from.
QA-visible copy changes
The two products' English differs in 17 places. Each choice is declared with a reason in
definitions-parity.test.tsand asserted against both products' transcribed copy, so the suitefails on an undeclared difference, a stale declaration, or a third wording we invented.
preamble is gone from four);
harmfulContentEntitiesas "Content categories" and its thresholdsas "Severity thresholds";
ipEntitiesas "Content types"; PII thresholds pluralized;LLM-as-judge
thresholdas "Strictness";SelfHarmas "Self-harm"; a new cost note.the Finland passport entity, which Flow renders as a raw value today.
prompt_injectionkeeps Agents' wording as the deliberate exception to the concision rule, becausethe Noma Security attribution is load-bearing.
Verification
tscclean,biomeclean, build clean, zod absent from all 35 emitted.d.tsunderdist/canvas/components/Guardrails/.Guardrailsdirectory: 19 files, 382 tests passing.Full package suite: 3020 passing, 10 failing, all ten the same pre-existing
localStorage-undefined failure incanvas/utils/Storage.test.ts,canvas/hooks/useStorageState.test.tsand
ap-chat/.../chat-message-content.test.tsx- none of which this PR touches. Node 24 ships anexperimental
localStorageglobal that isundefinedwithout--localstorage-fileand shadowshappy-dom's.
biomedoes flag four files in this folder that arrived with #1138 and are nowmain's, untouchedhere:
form-schema-builder.ts,utils.test.ts(organizeImports) andguardrail-builder.tsx,use-metadata-form-bridge.ts(useExhaustiveDependencies).Review questions
withGuardrailFolderMetadata,because resolving it needs each product's connections API (Agents pages
fetchResources, Flowcalls
getConnectionById). Do you want it inside the hook instead, as aresolveConnectionscallback?
of which changes what one product's users see. Happy to split any of them back out if a specific
string should stay as it is.